Make merkle tree leaf addition operations atomic #71

jimthematrix · 2024-09-17T17:29:05Z

fixes #21

Signed-off-by: Jim Zhang <[email protected]>

EnriqueL8

A view on the batch TX logic

EnriqueL8 · 2024-09-18T09:15:23Z

go-sdk/internal/sparse-merkle-tree/storage/sql.go

 	// the node's reference key (not the index) is used as the key to
 	// store the node in the DB
 	n := core.SMTNode{
 		RefKey: ref.Hex(),
 	}
-	err := s.p.DB().Table(s.nodesTableName).First(&n).Error
+	err := batchOrDb.Table(nodesTableName).First(&n).Error


I'm struggling why a read operation would need a batch transaction

here we want the query to hit the records that have been added in the batch, but have not been committed.

but if you are looking relying on getNode before inserting then you need to make sure to use the DB directly instead of batch

I feel batchOrDb should be dbTx here. this layer shouldn't care about that difference.

go-sdk/internal/sparse-merkle-tree/smt/merkletree.go

EnriqueL8 · 2024-09-18T09:23:40Z

go-sdk/internal/sparse-merkle-tree/smt/merkletree.go

+	if _, err := batch.GetNode(k); err == nil {
 		return nil, ErrNodeIndexAlreadyExists
 	}
-	err := mt.db.InsertNode(n)
+	err := batch.InsertNode(n)


This is where I don't understand the batch function where you get a node but the TX is not committed yet and then you are adding an insert to the same transaction

for example here, you would use db instead of batch for GetNode?

EnriqueL8

Thanks for the clarifications on the batching in GROM , makes sense to me

EnriqueL8

After a conversation with @jimthematrix , we can move to ON CONFLICT insert and if there is on conflict do nothing and rework the way the read works

Signed-off-by: Jim Zhang <[email protected]>

jimthematrix · 2024-09-18T14:05:53Z

@EnriqueL8 thanks for catching that. just added a commit to make the inserts `ON CONFLICT DO NOTHING" with a justification in the inline comments

Chengxuan

@jimthematrix core logic looks good to me

I think we should address the following before the merge:

test temp db file cleanup check
log rollback failures

I also made a suggestion for readability, which is to consider renaming batch to highlight it's a DB tx.

Chengxuan · 2024-09-18T05:32:17Z

go-sdk/integration-test/db_test.go

+}
+
+func (s *SqliteTestSuite) TearDownTest() {
+	os.Remove(s.dbfile.Name())


error check is regressed, this will cause removal failure due to the wrong file name not being captured.

Chengxuan · 2024-09-21T06:28:20Z

go-sdk/internal/sparse-merkle-tree/smt/merkletree.go

+	batch, err := mt.db.BeginBatch()
 	if err != nil {
 		return err
 	}
+	newRootKey, err := mt.addLeaf(batch, node, mt.rootKey, 0, path)
+	if err != nil {
+		log.L().Errorf("Error adding leaf node %s: %v, rolling back", node.Ref().Hex(), err)
+		_ = batch.Rollback()
+		return err
+	}
 	mt.rootKey = newRootKey

 	// update the root node index in the storage
-	err = mt.db.UpsertRootNodeIndex(mt.rootKey)
+	log.L().Infof("Upserting root node index to %s", mt.rootKey.Hex())
+	err = batch.UpsertRootNodeIndex(mt.rootKey)
 	if err != nil {
+		log.L().Errorf("Error upserting root node %s: %v, rolling back", mt.rootKey.Hex(), err)
+		_ = batch.Rollback()
+		return err
+	}
+	log.L().Infof("Committing batch operations for adding leaf node %s", node.Ref().Hex())
+	err = batch.Commit()
+	if err != nil {
+		log.L().Errorf("Error committing batch operations for adding leaf node %s: %v", node.Ref().Hex(), err)
+		_ = batch.Rollback()


This is the core logic implements Make merkle tree leaf addition operations atomic. Some readability suggestions. Here are the steps as per my understanding:

A single DB tx is created to ensure atomicity of multiple DB txs

Add a new leaf node as part of the started DB TX

Upsert MT root in the same DB TX

Commit only if both queries succeeded, otherwise rollback.

So I think it would improve the readability if batch is renamed to reflect it's a DB tx.

Also, we should log Rollback errors as warnings so leakage of DB txs can be monitored.

Chengxuan · 2024-09-21T06:39:51Z

go-sdk/integration-test/smt_test.go

+
+func testConcurrentInsertion(t *testing.T, alice *babyjub.PublicKey, values []int, salts []string) {
+	dbfile, db, _, _ := newSqliteStorage(t)
+	defer os.Remove(dbfile.Name())


Suggested change

defer os.Remove(dbfile.Name())

defer func() {

err := os.Remove(dbfile.Name())

assert.NoError(t, err)

}()

Chengxuan · 2024-09-21T06:42:21Z

go-sdk/integration-test/db_test.go

+}
+
+func (s *SqliteTestSuite) TearDownTest() {
+	os.Remove(s.dbfile.Name())


Suggested change

os.Remove(s.dbfile.Name())

if err := os.Remove(s.dbfile.Name()); err != nil {

panic(err);

}

Chengxuan · 2024-09-21T06:43:15Z

go-sdk/internal/sparse-merkle-tree/smt/smt_test.go

+}
+
+func (s *MerkleTreeTestSuite) TearDownTest() {
+	os.Remove(s.dbfile.Name())


Suggested change

os.Remove(s.dbfile.Name())

if err := os.Remove(s.dbfile.Name()); err != nil {

panic(err);

}

jimthematrix added 2 commits September 17, 2024 13:28

Make merkle tree leaf addition operations atomic

8c35ca2

Signed-off-by: Jim Zhang <[email protected]>

Test coverage

690278b

Signed-off-by: Jim Zhang <[email protected]>

EnriqueL8 reviewed Sep 18, 2024

View reviewed changes

go-sdk/internal/sparse-merkle-tree/smt/merkletree.go Show resolved Hide resolved

EnriqueL8 reviewed Sep 18, 2024

View reviewed changes

EnriqueL8 approved these changes Sep 18, 2024

View reviewed changes

EnriqueL8 requested changes Sep 18, 2024

View reviewed changes

Address review comments - onconflict donothing for inserts

b1a4d3b

Signed-off-by: Jim Zhang <[email protected]>

Merge branch 'main' into sql-atomicity

6532fdb

Chengxuan reviewed Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make merkle tree leaf addition operations atomic #71

Make merkle tree leaf addition operations atomic #71

jimthematrix commented Sep 17, 2024 •

edited

Loading

EnriqueL8 left a comment

EnriqueL8 Sep 18, 2024

jimthematrix Sep 18, 2024

EnriqueL8 Sep 18, 2024

EnriqueL8 Sep 18, 2024 •

edited

Loading

Chengxuan Sep 21, 2024

EnriqueL8 Sep 18, 2024 •

edited

Loading

EnriqueL8 Sep 18, 2024

EnriqueL8 left a comment

EnriqueL8 left a comment

jimthematrix commented Sep 18, 2024

Chengxuan left a comment

Chengxuan Sep 18, 2024

Chengxuan Sep 21, 2024

Chengxuan Sep 21, 2024

Chengxuan Sep 21, 2024

Chengxuan Sep 21, 2024

Chengxuan Sep 21, 2024

-	defer os.Remove(dbfile.Name())
+	defer func() {
+		err := os.Remove(dbfile.Name())
+		assert.NoError(t, err)
+	}()

-	os.Remove(s.dbfile.Name())
+	if err := os.Remove(s.dbfile.Name()); err != nil {
+		panic(err);
+	}

Make merkle tree leaf addition operations atomic #71

Are you sure you want to change the base?

Make merkle tree leaf addition operations atomic #71

Conversation

jimthematrix commented Sep 17, 2024 • edited Loading

EnriqueL8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnriqueL8 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnriqueL8 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnriqueL8 left a comment

Choose a reason for hiding this comment

EnriqueL8 left a comment

Choose a reason for hiding this comment

jimthematrix commented Sep 18, 2024

Chengxuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimthematrix commented Sep 17, 2024 •

edited

Loading

EnriqueL8 Sep 18, 2024 •

edited

Loading

EnriqueL8 Sep 18, 2024 •

edited

Loading